no-jira: Validate AWS resource tag keys with aws: prefix#2183
no-jira: Validate AWS resource tag keys with aws: prefix#2183openshift-merge-bot[bot] merged 3 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@patrickdillon: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/hold |
|
Hello @patrickdillon! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
do-not-merge/hold
openshift-merge-robot
added
the
needs-rebase
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
the
lifecycle/stale
|
@patrickdillon Did you intend to move this PR forward? |
Yes. I see now the dependent work has merged. So I can rebase this; I am tied up with a conference this week so I will try to get this once I'm back next week. |
|
@patrickdillon Just a reminder to move this one along when you have a moment |
patrickdillon
force-pushed
the
aws-tag-prefix
branch
from
9ae0e4a to
aee6105
Compare
openshift-merge-robot
removed
the
needs-rebase
openshift-ci
Bot
added
size/L
|
/remove-lifecycle stale |
openshift-ci
Bot
removed
the
lifecycle/stale
|
/hold cancel @JoelSpeed finally ready, thanks! |
openshift-ci
Bot
removed
the
do-not-merge/hold
| value: value* | ||
| type: AWS | ||
| expectedStatusError: "invalid AWS resource tag value. The string can contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', '@'" | ||
| - name: Should not be able to create an aws resourcetag with aws prefix in key |
There was a problem hiding this comment.
Please also add a ratcheting test (see readme in the tests dir) to prove that existing values are not affected by this change. You should show that
- Existing entries persist when a new item is added to the list
- Additional bad entries are not allowed when there is an existing bad entry
- The bad entry can be updated to a valid entry
There was a problem hiding this comment.
Ok, I added the ratcheting validation tests, but am stuck on the first one (the other two work ok). In order to allow existing bad values, I updated the validation in 34f8d72 but apparently this is invalid, as the tests fail with:
Invalid value: "self == oldSelf || !self.startsWith('aws:')": oldSelf cannot be used on the uncorrelatable portion of the schema within spec.validation.openAPIV3Schema.properties[status].properties[platformStatus].properties[aws].properties[resourceTags]
I see in these k8s docs that this means the rule cannot be applied:
Errors will be generated on CRD writes if a schema node contains a transition rule that can never be applied, e.g. "oldSelf cannot be used on the uncorrelatable portion of the schema within path".
I'm not sure how to resolve this issue. Any insight?
There was a problem hiding this comment.
@JoelSpeed claude helped me figure out that I needed to change ResourceTags to listType=map, but now the actual validation of the aws: prefix does not seem to be working
[config.openshift.io/v1, Resource=infrastructures][ClusterProfiles=Hypershift,SelfManagedHA][FeatureSet="Default"][FeatureGate=-AWSClusterHostedDNSInstall][File=0000_10_config-operator_01_infrastructures-Default.crd.yaml] Infrastructure On Update [It] Should not be able to create an aws resourcetag with aws prefix in key
/Users/padillon/go/src/github.com/openshift/api/tests/generator.go:345
[FAILED] Expected an error, got nil
In [It] at: /Users/padillon/go/src/github.com/openshift/api/tests/generator.go:321 @ 10/14/25 22:00:52.111
Stuck on this.
There was a problem hiding this comment.
I went ahead and side-stepped the issue by validating for immutability. IMHO that makes more sense, as post-install updates are not supported. On the other hand, I'm not sure if there are issues with making a field immutable post-GA; the intention was always for these fields to be immutable, it just wasn't validated.
There was a problem hiding this comment.
We still need a ratcheting test to show that anyone who already has a value with aws: in the prefix won't be broken by this change
There was a problem hiding this comment.
Oh right, that makes sense: so we don't brick the rest of the aws platform status from being updated. Because I made resourceTags immutable, we can't cover the three expected cases in your original message. Does the check added in d582954 look good/sufficient? Or what else would you have in mind.
There was a problem hiding this comment.
Yep, that covers it, thanks
|
/hold |
openshift-ci
Bot
added
do-not-merge/hold
|
@JoelSpeed these fields are supposed to be immutable. Should i just add validation to enforce that? |
|
/retest-required |
|
@JoelSpeed as much as I'd like to get this merged, I view this as non critical so would lean toward waiting until the ack requirement is removed. not a strong opinion though... |
|
/verified e2e-aws-ovn |
|
@patrickdillon: The DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/verified by e2e-aws-ovn |
openshift-ci-robot
added
the
verified
|
@patrickdillon: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
This needs regenerating now that the new OKD featureset is in |
patrickdillon
force-pushed
the
aws-tag-prefix
branch
from
f280b2c to
db2a33a
Compare
openshift-ci-robot
removed
the
verified
Pushed! Watching tests in case I missed anything. |
AWS docs indicate that tag keys cannot be prefix with aws:. See: https://docs.aws.amazon.com/directoryservice/latest/devguide/API_Tag.html Using this key prefix leads to an AWS API error indicating the prefix is reserved for AWS system usage. This commit adds API validation and ratecheting tests, as the key was previously allowed. Adds validation to enforce immutability on AWS resourcetags, in the same manner as Azure & GCP. Updating resourcetags post-install is not supported.
PROTO_OPTIONAL=true make update
patrickdillon
force-pushed
the
aws-tag-prefix
branch
from
db2a33a to
244900a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml (1)
1671-1689: Fix two regex and prefix validation issues in AWS tag key rules.The allowed-characters regex
^[0-9A-Za-z_.:/=+-@ ]+$unintentionally creates a character range with+-@, allowing characters like,,;,<,>,?that AWS does not permit. Additionally, the reserved prefix check only validates lowercaseaws:but AWS restricts the prefix regardless of case (e.g.,AWS:,Aws:are also reserved).Proposed fixes
- message: invalid AWS resource tag key. The string can contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', '@' - rule: self.matches('^[0-9A-Za-z_.:/=+-@ ]+$') + rule: self.matches('^[0-9A-Za-z_.:/=+@ -]+$') - message: the prefix 'aws:' is reserved for AWS system usage and cannot be used at the beginning of a key - rule: '!self.startsWith(''aws:'')' + rule: '!self.lowerAscii().startsWith(''aws:'')'payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml (1)
1385-1419: AWS tag key/value regex likely allows unintended characters due to+-@range.On Line 1399 (and similarly Line 1418), the character class
...=+-@...will be interpreted as a range from+to@in RE2, which admits extra punctuation (e.g.,, ; < > ?) that the message explicitly says should be rejected—weakening the validation and potentially still causing AWS API errors. The newaws:prefix ban itself (Lines 1400-1402) looks good.Proposed fix (escape/move '-' so it's a literal)
- rule: self.matches('^[0-9A-Za-z_.:/=+-@ ]+$') + rule: self.matches('^[0-9A-Za-z_.:/=+ :@-]+$') ... - rule: self.matches('^[0-9A-Za-z_.:/=+-@ ]+$') + rule: self.matches('^[0-9A-Za-z_.:/=+ :@-]+$')payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml (1)
1471-1505: Fix the AWS tag key/value regex:-is currently a range, not a literal.
self.matches('^[0-9A-Za-z_.:/=+-@ ]+$')likely interprets+-@as an ASCII range, allowing many characters beyond the message/docs (e.g.,,,?, etc.), which can reintroduce AWS API errors.Proposed fix
- rule: self.matches('^[0-9A-Za-z_.:/=+-@ ]+$') + rule: self.matches('^[0-9A-Za-z_.:/=+\-@ ]+$') ... - rule: self.matches('^[0-9A-Za-z_.:/=+-@ ]+$') + rule: self.matches('^[0-9A-Za-z_.:/=+\-@ ]+$')payload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yaml (1)
1189-1205: Fix two tag validation bugs: (1) regex character class range, (2) case-insensitive AWS prefix check.The regex
^[0-9A-Za-z_.:/=+-@ ]+$contains+-@, which RE2 interprets as a character range from+to@(code points 43–64), unintentionally allowing;,<,>,?, and other characters not documented. Escape the dash:+\-@.Additionally, AWS rejects tag keys starting with
aws:in any case variation (AWS, Aws, aWs, etc.). The current rule!self.startsWith('aws:')only blocks lowercase; use!self.lowerAscii().startsWith('aws:')instead.This pattern appears in multiple infrastructure CRD files:
0000_10_config-operator_01_infrastructures-Default.crd.yaml0000_10_config-operator_01_infrastructures-OKD.crd.yaml0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlProposed diffs
- rule: self.matches('^[0-9A-Za-z_.:/=+-@ ]+$') + rule: self.matches('^[0-9A-Za-z_.:/=+\-@ ]+$')- rule: '!self.startsWith(''aws:'')' + rule: '!self.lowerAscii().startsWith(''aws:'')'config/v1/types_infrastructure.go (1)
583-602: Fix the character class regex and prefix validation in AWS tag rules.The regex pattern
[0-9A-Za-z_.:/=+-@ ]uses an unintended range:+-@(ASCII 43–64) expands to include;,<,>,?, and,— characters not documented as allowed. Escape the hyphen as\-in both Key and Value validation rules.Additionally, AWS documentation specifies that the
aws:prefix reservation applies to "any case combination," but the currentstartsWith('aws:')check only rejects lowercase. Consider using a case-insensitive check (e.g.,self.lowerAscii().startsWith('aws:')) to rejectAWS:,Aws:, etc.Proposed diff for regex fix
// +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:MaxLength=128 - // +kubebuilder:validation:XValidation:rule=`self.matches('^[0-9A-Za-z_.:/=+-@ ]+$')`,message="invalid AWS resource tag key. The string can contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', '@'" + // +kubebuilder:validation:XValidation:rule=`self.matches('^[0-9A-Za-z_.:/=+\-@ ]+$')`,message="invalid AWS resource tag key. The string can contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', '@'" // +kubebuilder:validation:XValidation:rule=`!self.startsWith('aws:')`,message="the prefix 'aws:' is reserved for AWS system usage and cannot be used at the beginning of a key" // +required Key string `json:"key"` @@ // +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:MaxLength=256 - // +kubebuilder:validation:XValidation:rule=`self.matches('^[0-9A-Za-z_.:/=+-@ ]+$')`,message="invalid AWS resource tag value. The string can contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', '@'" + // +kubebuilder:validation:XValidation:rule=`self.matches('^[0-9A-Za-z_.:/=+\-@ ]+$')`,message="invalid AWS resource tag value. The string can contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', '@'" // +required Value string `json:"value"`
🤖 Fix all issues with AI agents
In
@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml:
- Around line 1510-1516: Add optionalOldSelf: true under the
x-kubernetes-validations for this array and replace the existing rule with one
that allows CREATE when oldSelf is absent and enforces strict equality on
updates: use a rule equivalent to '!oldSelf.hasValue() || self == oldSelf' so
oldSelf is optional and duplicate-order differences are detected and prevented.
In
@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml:
- Around line 1686-1689: The CEL validation currently uses
"!self.startsWith('aws:')" which only blocks the lowercase prefix; update the
rule to perform a case-insensitive check by replacing that expression with
"!self.lowerAscii().startsWith('aws:')" (keep the surrounding rule key name
"rule" and the existing message text or adjust it if needed to reflect
case-insensitive matching).
🧹 Nitpick comments (1)
payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml (1)
1423-1429:resourceTags“immutability” rule allows duplicate-count changes; confirm that’s acceptable.The set-style rule on Lines 1426-1429 allows changing multiplicity (e.g.,
[tagA]→[tagA, tagA]and vice versa) because membership checks don’t account for counts. If duplicates are meaningless, consider also enforcing unique keys (or switching to list-typemapkeyed bykeyif that’s safe for compatibility).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (38)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AWSClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AWSDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AzureClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AzureDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/DualReplica.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/DyanmicServiceEndpointIBMCloud.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/GCPClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/HighlyAvailableArbiter+DualReplica.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/HighlyAvailableArbiter.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/NutanixMultiSubnets.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/OnPremDNSRecords.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/VSphereHostVMGroupZonal.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/VSphereMultiNetworks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AWSClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AWSDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AzureClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AzureDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/DualReplica.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/DyanmicServiceEndpointIBMCloud.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/GCPClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/HighlyAvailableArbiter+DualReplica.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/HighlyAvailableArbiter.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/NutanixMultiSubnets.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/OnPremDNSRecords.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/VSphereHostVMGroupZonal.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiNetworks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (14)
config/v1/tests/infrastructures.config.openshift.io/AAA_ungated.yamlconfig/v1/types_infrastructure.goconfig/v1/zz_generated.swagger_doc_generated.goopenapi/generated_openapi/zz_generated.openapi.gopayload-manifests/crds/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- config/v1/zz_generated.swagger_doc_generated.go
- openapi/generated_openapi/zz_generated.openapi.go
🔇 Additional comments (32)
payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml (3)
1713-1717: LGTM! Array-level immutability validation is correctly implemented.The validation rule correctly enforces that
resourceTagscannot be added, removed, or modified after installation by checking bidirectional set equality. Since the comparison includes the entire object (bothkeyandvalue), this properly prevents modifications to tag values as well as keys.The rule only applies to updates (when
oldSelfexists), allowingresourceTagsto be set during initial installation as intended.
1746-1749: LGTM! Object-level presence check complements the array-level validation.This validation correctly prevents the
resourceTagsfield from being added or removed after installation. Combined with the array-level immutability check (lines 1713-1717), this provides complete protection:
- Field presence is immutable (this rule)
- Array contents are immutable (array-level rule)
Both validations only apply to updates, properly allowing
resourceTagsto be configured during initial installation.
1676-1676: LGTM! Description clearly documents the constraint.The updated description accurately reflects the new validation rule, informing users that tag keys cannot start with 'aws:'. This complements the other validation rules (character set and length) to fully document AWS tag key requirements.
payload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yaml (3)
1193-1205: LGTM! AWS prefix validation correctly implemented.The validation logic correctly prevents tag keys from using the reserved
aws:prefix. The error message clearly explains the constraint, and the validation order (allowed characters first, then prefix check) is appropriate.
1229-1232: LGTM! Array-level immutability correctly enforced.The bidirectional subset check (
self.all(x, x in oldSelf) && oldSelf.all(x, x in self)) correctly ensures that resourceTags cannot be modified, added to, or removed from after installation. The validation message is clear and appropriate.
1261-1263: LGTM! Object-level field presence validation correctly implemented.The validation
has(oldSelf.resourceTags) == has(self.resourceTags)correctly prevents adding or removing the entire resourceTags field after installation. This complements the array-level immutability check by ensuring the field itself cannot be added/removed post-install.config/v1/tests/infrastructures.config.openshift.io/AAA_ungated.yaml (7)
1840-1867: LGTM! AWS prefix validation test is well-structured.The test correctly verifies that tag keys starting with
aws:are rejected with the appropriate error message. The test scenario and expected error align with the CRD validation rules.
1868-1893: LGTM! Tag modification test correctly validates immutability.This test properly verifies that modifying an existing tag value is rejected by the array-level immutability validation. The expected error message matches the CRD constraint.
1894-1920: LGTM! Tag addition test correctly validates immutability.This test properly verifies that adding new tags to an existing resourceTags array is rejected. The bidirectional subset check in the CRD will catch this via
self.all(x, x in oldSelf)failing when a new tag is present.
1921-1945: LGTM! Tag removal test correctly validates immutability.This test properly verifies that removing tags from an existing resourceTags array is rejected. The bidirectional subset check will catch this via
oldSelf.all(x, x in self)failing when a tag is removed.
1946-1966: LGTM! Adding resourceTags field test correctly validates installation-time configurability.This test properly verifies that the resourceTags field cannot be added to an existing empty aws object post-installation. The object-level validation correctly catches this field presence change.
1967-1988: LGTM! Removing resourceTags field test correctly validates installation-time configurability.This test properly verifies that the resourceTags field cannot be removed from an existing aws object post-installation. The object-level validation correctly catches this field presence change.
1989-2032: LGTM! Ratcheting validation test correctly verifies backward compatibility.This test properly verifies that existing installations with invalid data (created before the aws: prefix validation) can still update other fields without being blocked by the new validation. This is essential for smooth upgrades and follows Kubernetes validation best practices.
payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml (2)
1710-1717:resourceTagsimmutability rule: confirm intended semantics withx-kubernetes-list-type: atomic.The set-equality style rule will permit reordering while preventing add/remove/modify (assuming object equality behaves as expected), which is often desirable. With
x-kubernetes-list-type: atomic, double-check that allowing reorder is OK (vs. strictself == oldSelf) and thatoldSelfbehavior on create/update can’t cause unexpected rejections.
1745-1749: Install-time-only gating forresourceTagspresence looks right.
has(oldSelf.resourceTags) == has(self.resourceTags)cleanly prevents adding/removing the field post-install, and complements the per-list immutability rule.payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml (3)
1676-1689: LGTM! Correctly validates AWS reserved tag key prefix.The validation rule
!self.startsWith('aws:')correctly prevents users from using the AWS-reserved prefix. AWS reserves the lowercaseaws:prefix for system tags, so the case-sensitive check is appropriate. The error message clearly explains the constraint.
1713-1717: LGTM! Correct immutability validation for resourceTags array.The bidirectional containment check
self.all(x, x in oldSelf) && oldSelf.all(x, x in self)correctly ensures set equality, preventing both additions and removals after installation. This enforces the installation-time-only configuration requirement.
1746-1749: LGTM! Field-level presence immutability complements array-level validation.The validation
has(oldSelf.resourceTags) == has(self.resourceTags)correctly prevents adding or removing theresourceTagsfield after installation. Combined with the array-level immutability validation (lines 1713-1717), this provides complete protection for the installation-time-only configuration requirement.payload-manifests/crds/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yaml (3)
1385-1402: LGTM! Validation for reserved AWS tag key prefix.The description update (line 1390) and the CEL validation rule (lines 1400-1402) correctly implement the AWS reserved prefix constraint. The
startsWith('aws:')check aligns with AWS's case-sensitive handling of this reserved prefix.
1426-1429: LGTM! Immutability enforcement for resourceTags list.The CEL rule
self.all(x, x in oldSelf) && oldSelf.all(x, x in self)correctly enforces that the list contents cannot change after installation. Combined with the atomic list type, this prevents any modifications to the tag list.
1458-1460: LGTM! Parent-level immutability for resourceTags field presence.This validation
has(oldSelf.resourceTags) == has(self.resourceTags)ensures the resourceTags field itself cannot be added or removed post-installation, complementing the list-level immutability check above.payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml (1)
1457-1461: Verify thehas(oldSelf.resourceTags) == has(self.resourceTags)rule doesn’t block initial population.Lines 1458-1460 prevent adding/removing the
resourceTagsfield after it’s absent/present. Ifstatus.platformStatus.aws.resourceTagsis populated via a subsequent status update (rather than being present on initial create), this rule would reject that update. Please confirm the install-time flow setsresourceTagsin a way that still passes this rule.payload-manifests/crds/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yaml (4)
1390-1390: Documentation update looks good.The description clearly communicates the 'aws:' prefix restriction to users.
1426-1429: LGTM! Immutability validation is correctly implemented.The bidirectional containment check (
self.all(x, x in oldSelf) && oldSelf.all(x, x in self)) correctly enforces that resourceTags cannot be modified after installation. This pattern is consistent with similar validations for Azure and GCP platforms in this CRD.
1458-1460: LGTM! Platform-level immutability validation is correct.The rule
has(oldSelf.resourceTags) == has(self.resourceTags)correctly enforces that the resourceTags field can only be configured during installation and cannot be added or removed afterward. Combined with the array-level validation at lines 1426-1429, this ensures complete immutability of resourceTags after installation.
1400-1402: Update validation rule to use case-insensitive prefix check.The
startsWith('aws:')check only blocks the lowercase 'aws:' prefix. AWS rejects tag keys beginning with any case variation of the prefix, including 'AWS:', 'Aws:', and others. Use a case-insensitive comparison instead, such aslower(self).startsWith('aws:').payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml (1)
1544-1549:has(oldSelf.resourceTags) == has(self.resourceTags)matches the “install-time only” intent.This cleanly prevents adding/removing
resourceTagspost-install (including distinguishing “unset” vs “set to empty”), and pairs well with the per-list immutability rule.payload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yaml (1)
1229-1232: Install-time gating + immutability rules forresourceTagsare correctly implemented.Transition rules using
oldSelfare skipped on CREATE and only evaluated on UPDATE. The two rules here—self.all(x, x in oldSelf) && oldSelf.all(x, x in self)(line 1229) andhas(oldSelf.resourceTags) == has(self.resourceTags)(line 1261)—will allow initial configuration during installation and enforce immutability on subsequent updates, exactly as intended.config/v1/types_infrastructure.go (1)
531-553: AWSresourceTagsinstall-time gating + immutability annotations are correctly implemented.The combination of:
- type-level presence constraint (
has(oldSelf.resourceTags) == has(self.resourceTags)) and- list-level immutability rule (
self.all(x, x in oldSelf) && oldSelf.all(x, x in self))correctly enforces the "install-time only" requirement. The CEL functions used (
has,all,in,startsWith) are available in Kubernetes x-kubernetes-validations (beta since K8s 1.25, GA since K8s 1.29), with no compatibility concerns for OpenShift's supported versions.payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yaml (3)
1469-1488: LGTM! AWS reserved prefix validation correctly implemented.The
startsWith('aws:')check properly enforces AWS's reserved tag key prefix constraint. The case-sensitive check is correct since AWS treats tag keys as case-sensitive and specifically reserves the lowercaseaws:prefix.
1510-1516: LGTM! Immutability validation is correctly enforced.The bidirectional containment check
self.all(x, x in oldSelf) && oldSelf.all(x, x in self)correctly enforces that the resourceTags array cannot be modified after installation. This pattern appropriately works with theatomiclist type.
1544-1548: LGTM! Field presence validation correctly complements array immutability.The
has(oldSelf.resourceTags) == has(self.resourceTags)rule ensures that the resourceTags field cannot be added or removed post-installation, working in conjunction with the array-level immutability validation to fully enforce install-time-only configuration.
| type: array | ||
| x-kubernetes-list-type: atomic | ||
| x-kubernetes-validations: | ||
| - message: resourceTags are immutable and may only | ||
| be configured during installation | ||
| rule: self.all(x, x in oldSelf) && oldSelf.all(x, | ||
| x in self) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Kubernetes CRD x-kubernetes-validations CEL oldSelf when defined create update
💡 Result:
Short answer
- oldSelf represents the prior resource state and is only populated for UPDATE (when an old object exists). [1]
- On CREATE there is no old object, so oldSelf is not present by default. [1][2]
- If you need to evaluate a transition rule on CREATE (or handle a missing old value), use the optionalOldSelf feature; then oldSelf becomes a CEL Optional (None when missing) and you must check oldSelf.hasValue() and unwrap with oldSelf.value(). [2][3]
Practical notes
- You may only enable optionalOldSelf when your rule references oldSelf; otherwise it won’t be set. [2]
- Typical pattern for “immutable once set”:
- Require the field on create, then use rule self == oldSelf for updates.
- Or use optionalOldSelf and guard: !oldSelf.hasValue() || self == oldSelf. [3][2]
Sources
- Kubernetes CEL docs (x-kubernetes-validations, self / oldSelf explanation). [1]
- apiextensions description of optionalOldSelf and Optional handling. [2]
- Kubernetes blog examples showing create vs update/immutability patterns. [3]
🌐 Web query:
Kubernetes CEL validation rules immutable arrays best practices
💡 Result:
Concise best practices for making arrays (lists) immutable or append‑only with Kubernetes CEL validation rules:
- Use oldSelf/self comparisons to block changes: for an entirely immutable field use rule "self == oldSelf". [1][2]
- Allow initial empty → then immutable: combine type-scoped and field-scoped rules: "!has(oldSelf.field) || has(self.field)" plus "self == oldSelf" to permit initial set but prevent later changes. [1]
- For append‑only lists: assert every old element still exists in new list: "oldSelf.all(x, x in self)". Combine with "!has(oldSelf.field) || has(self.field)" if initially optional. Example shown in upstream docs. [1]
- For lists of objects, compare identity keys (e.g., name or id) rather than whole objects if elements may carry mutable subfields; enforce that identities are not removed and optionally that identities are not reordered. Use "oldSelf.all(x, x in self)" on identity projection (e.g., oldSelf.items.all(i, i.id in self.items.id)). If you need order independence, use membership-based checks rather than positional equality. [1][2]
- Set schema bounds to keep CEL cost predictable: add maxItems, maxProperties, maxLength to arrays/maps/strings. This reduces estimated and runtime CEL cost and prevents rule rejection. [3]
- Enforce uniqueness explicitly (if required) using CEL (e.g., check counts or membership) since lists are not automatically unique. [2][3]
- Keep expressions simple and avoid O(n^2) patterns over unbounded lists; prefer membership checks and bounded iterations. Rely on explicit size limits to allow slightly more complex checks. [3]
- Test expressions in the CEL Playground and iterate; validate both CREATE and UPDATE flows (use oldSelf variable on updates). Also include helpful message text in x-kubernetes-validations for user-friendly rejections. [2][4]
Key references:
- Enforce CRD Immutability with CEL (examples for immutable fields and append-only lists). [1]
- Kubernetes CEL docs (language, available variables: self/oldSelf, best practices). [2]
- CRD validation rules / cost limits guidance (use maxItems/maxLength to control estimated & runtime cost). [3]
- ValidatingAdmissionPolicy docs / CEL testing (playground, object/oldObject variables). [4]
Sources:
[1] Kubernetes blog: Enforce CRD Immutability with CEL.
[2] Kubernetes docs: Common Expression Language in Kubernetes.
[3] Kubernetes blog: CRD Validation Rules Beta / cost limits and schema guidance.
[4] Kubernetes docs: Validating Admission Policy (CEL usage and testing).
🌐 Web query:
Kubernetes CRD CEL oldSelf undefined on create validation rules
💡 Result:
Short answer: oldSelf being undefined on CREATE is expected — transition rules are only given an old value when an existing object is being updated. To handle creation you must either (a) make the rule tolerate a missing oldSelf (guard with has(oldSelf) / oldSelf.hasValue()), or (b) set optionalOldSelf: true on the validation so oldSelf becomes an Optional you can check/unpack. [1][2][3]
Examples:
-
Guarding without optionalOldSelf:
- rule: "!has(oldSelf) || (oldSelf.spec.foo == self.spec.foo)"
- message: "foo is immutable"
-
Using optionalOldSelf:
- x-kubernetes-validations:
- rule: "oldSelf.hasValue() ? oldSelf.value().spec.foo == self.spec.foo : true"
message: "foo is immutable"
optionalOldSelf: true
- rule: "oldSelf.hasValue() ? oldSelf.value().spec.foo == self.spec.foo : true"
- x-kubernetes-validations:
References:
- CRD validation rules / transition rules (docs + examples). [1]
- optionalOldSelf explanation (apiextensions field). [2]
- CEL immutability examples showing guards like !has(oldSelf.field) || has(self.field). [3]
If you want, paste your CRD validation snippet and I’ll show the minimal fix.
🏁 Script executed:
cat -n payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml | sed -n '1500,1530p'Repository: openshift/api
Length of output: 2362
🏁 Script executed:
cat -n payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml | sed -n '1510,1520p'Repository: openshift/api
Length of output: 856
🏁 Script executed:
cat -n payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml | sed -n '1512,1516p'Repository: openshift/api
Length of output: 433
Add optionalOldSelf: true and fix rule to prevent creation failure and duplicate vulnerability.
The rule will fail on CREATE operations because oldSelf is undefined (and not marked as optional). Additionally, the current rule allows duplicates: [a, b] and [a, a, b, b] both pass the membership check.
Fix:
x-kubernetes-validations:
- message: resourceTags are immutable and may only be configured during installation
rule: '!oldSelf.hasValue() || self == oldSelf'
optionalOldSelf: trueThis permits creation (when oldSelf is absent) and enforces strict immutability (order-sensitive, no duplicates) on updates.
🤖 Prompt for AI Agents
In
@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml
around lines 1510 - 1516, Add optionalOldSelf: true under the
x-kubernetes-validations for this array and replace the existing rule with one
that allows CREATE when oldSelf is absent and enforces strict equality on
updates: use a rule equivalent to '!oldSelf.hasValue() || self == oldSelf' so
oldSelf is optional and duplicate-order differences are detected and prevented.
| - message: the prefix 'aws:' is reserved for | ||
| AWS system usage and cannot be used at the | ||
| beginning of a key | ||
| rule: '!self.startsWith(''aws:'')' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
AWS resource tag keys "aws:" prefix case-sensitive restriction documentation
💡 Result:
Short answer: AWS reserves the "aws:" prefix for service use and you may not create tag keys that begin with that prefix in any case (e.g., "aws:", "AWS:", or other upper/lowercase variants). AWS docs also state tag keys are case‑sensitive (so keys like "CostCenter" and "costcenter" are distinct) but the reserved prefix rule applies regardless of case. (docs.aws.amazon.com)
Citations:
The validation rule must block the "aws:" prefix case-insensitively to match AWS requirements.
The current CEL rule !self.startsWith('aws:') only blocks the lowercase "aws:" prefix. However, AWS reserves the "aws:" prefix in all cases—including "AWS:", "Aws:", and other uppercase variants. The validation must be updated to perform a case-insensitive check to align with AWS tag key restrictions.
Update the rule to: !self.lowerAscii().startsWith('aws:')
🤖 Prompt for AI Agents
In
@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml
around lines 1686 - 1689, The CEL validation currently uses
"!self.startsWith('aws:')" which only blocks the lowercase prefix; update the
rule to perform a case-insensitive check by replacing that expression with
"!self.lowerAscii().startsWith('aws:')" (keep the surrounding rule key name
"rule" and the existing message text or adjust it if needed to reflect
case-insensitive matching).
|
unit tests passed |
|
/lgtm |
openshift-ci-robot
added
the
verified
|
@JoelSpeed: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/override ci/prow/e2e-aws-serial-techpreview |
|
@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/e2e-aws-serial, ci/prow/e2e-aws-serial-techpreview DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@patrickdillon: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
6 participants
AWS docs indicate that tag keys cannot be prefix with aws:. See:
https://docs.aws.amazon.com/directoryservice/latest/devguide/API_Tag.html
Using this key prefix leads to an AWS API error indicating the prefix
is reserved for AWS system usage. This commit adds API validation
and ratecheting tests, as the key was previously allowed.
This was originally included in #2124 but cannot land due to a bug addressed by openshift/kubernetes#2167, so
Depends on #2124
Depends on openshift/kubernetes#2167